Skip to content

Fix BitList positional operations after elements are removed - #16578

Open
waw666waw666 wants to merge 1 commit into
apache:3.3from
waw666waw666:fix/bitlist-positional-ops
Open

waw666waw666 wants to merge 1 commit into
apache:3.3from
waw666waw666:fix/bitlist-positional-ops

Conversation

@waw666waw666

Copy link
Copy Markdown

What is the purpose of the change?

BitList stores the live elements of originList as set bits in rootSet. Once an element has been removed, for example an invoker filtered out by a state router (the normal state of a routed invoker list), a position in the list is no longer the same as an index in originList. get(int), remove(int) and indexOf handle this already. These java.util.List operations don't:

  • subList(from, to) clears bits [0, from) and [to, ...) of rootSet. It treats list positions as originList indexes, so it returns elements that are not in the list and drops elements that are.
  • new BitList<>(bitList) sets bits [0, bitList.size()) instead of copying rootSet. The copy brings back removed elements and loses live ones. It also shares the source's tailList, so changes to the copy leak into the source. With a tail list, iterating the copy can throw IndexOutOfBoundsException.
  • BitListIterator:
    • remove() does not reset its state, so a second remove() doesn't throw IllegalStateException. It silently removes another element instead.
    • After removing the element returned by next(), the cursor still points at that element, so hasPrevious() and previous() still return it.
    • nextIndex() returns size - 1 at the end of the list.
    • listIterator(index) does not check its bounds and doesn't position correctly inside the tail list.

Reproduction on current 3.3 (2d9730c):

BitList<String> invokers = new BitList<>(Arrays.asList("A", "B", "C", "D", "E"));
invokers.remove("B");                 // [A, C, D, E]
invokers.subList(1, 3);               // [C]           expected [C, D]
new BitList<>(invokers);              // [A, B, C, D]  expected [A, C, D, E]

ListIterator<String> it = invokers.listIterator();
it.next();
it.remove();                          // [C, D, E]
it.hasPrevious();                     // true          expected false
it.remove();                          // no exception, list is now [D, E]

Code that treats a routed BitList as a plain List can therefore get invokers that were routed away, or lose valid ones. That includes RouterSnapshotNode, which prints subList(0, 5) of each node's invokers, and custom routers or load balancers.

What this PR changes:

  • Adds a bitIndexOf(int) helper that maps a list position to its originList index. get(int) and remove(int) now use it instead of repeating the loop, and remove(int) rejects negative indexes like get(int) does.
  • subList translates from/to through rootSet, copies only the needed part of the tail list, and checks its bounds the way List.subList requires.
  • The copy constructor copies rootSet and tailList when the source is a BitList.
  • BitListIterator keeps the originList index of the last returned element. remove() clears exactly that bit (no more O(n) scan per removal), steps the cursor back when the removed element was returned by next(), and resets its state. nextIndex() is always previousIndex() + 1. The constructor positions the cursor directly, including inside the tail list.

removeIf and plain forward iteration, which the routers use, behave as before. They just no longer scan the bit set on every remove().

Tests: BitListTest gets four new tests: subList over every range of a list with removed elements and a tail, the copy constructor and its independence from the source, ListIterator.remove() semantics, and a seeded randomized comparison of BitList.listIterator(i) against ArrayList (next/previous/remove/nextIndex/previousIndex, with and without tail elements). All four fail on 3.3 and pass with this change. mvn -pl dubbo-cluster test passes (314 tests), and so do spotless:check and checkstyle:check.

Checklist

  • Make sure there is a GitHub_issue field for the change. (No existing issue; the problem and reproduction are described above.)
  • Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • Write necessary unit-test to verify your logic correction. If the new feature or significant change is committed, please remember to add sample in dubbo samples project.
  • Make sure gitHub actions can pass. Why the workflow is failing and how to fix it?

BitList keeps the live elements of originList as set bits in rootSet, so
once an element has been removed (e.g. an invoker filtered out by a router)
a position in the list no longer equals an index in originList. Several
java.util.List operations ignored that:

- subList(from, to) cleared bits [0, from) and [to, ...) of rootSet,
  treating list positions as originList indexes, so it returned elements
  that are not in the list and dropped ones that are.
- new BitList<>(bitList) set bits [0, bitList.size()) instead of copying
  rootSet, so the copy resurrected removed elements and lost others, and it
  shared the source's tailList, so changes to one leaked into the other.
- BitListIterator.remove() did not reset its state, so a second remove()
  silently removed another element, and after removing the element returned
  by next() it kept the cursor on that element, so hasPrevious()/previous()
  still returned it. nextIndex() returned size - 1 at the end of the list,
  and listIterator(index) did not position correctly inside the tail list.

Translate list positions through rootSet with a shared bitIndexOf helper,
copy the source state in the copy constructor, and track the bit of the
last returned element in the iterator. Add regression tests, including a
randomized comparison of BitList's ListIterator against ArrayList's.
@codecov-commenter

codecov-commenter commented Nov 7, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 93.05556% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 61.02%. Comparing base (2d9730c) to head (0bb6fa0).

Files with missing lines Patch % Lines
...apache/dubbo/rpc/cluster/router/state/BitList.java 93.05% 2 Missing and 3 partials ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##                3.3   #16578      +/-   ##
============================================
+ Coverage     61.00%   61.02%   +0.01%     
- Complexity       15    11760   +11745     
============================================
  Files          1953     1953              
  Lines         89286    89285       -1     
  Branches      13478    13472       -6     
============================================
+ Hits          54471    54485      +14     
+ Misses        29213    29201      -12     
+ Partials       5602     5599       -3     
Flag Coverage Δ
integration-tests-java21 32.07% <26.76%> (-0.02%) ⬇️
integration-tests-java8 32.21% <26.76%> (+0.01%) ⬆️
samples-tests-java21 32.13% <32.39%> (-0.05%) ⬇️
samples-tests-java8 29.71% <32.39%> (-0.08%) ⬇️
unit-tests-java11 59.29% <92.95%> (+0.03%) ⬆️
unit-tests-java17 58.72% <92.95%> (+0.02%) ⬆️
unit-tests-java21 58.72% <92.95%> (-0.02%) ⬇️
unit-tests-java25 58.66% <92.95%> (-0.01%) ⬇️
unit-tests-java8 59.29% <93.05%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants